-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
datadog: sample_rate omitted by default #1
Conversation
// Default: 1.0 | ||
DatadogSampleRate float32 `json:"datadog-sample-rate"` | ||
// Default: use a dynamic rate instead | ||
DatadogSampleRate *float32 `json:"datadog-sample-rate",omitempty` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is configuration ever printed/serialized after being parsed into this struct
? If so, non-nil
DatadogSampleRate
would probably render as the pointer value, which is not what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some thoughts and suggestions
internal/ingress/controller/nginx.go
Outdated
@@ -1084,37 +1074,65 @@ ratio = {{ .OtelSamplerRatio }} | |||
parent_based = {{ .OtelSamplerParentBased }} | |||
` | |||
|
|||
func datadogOpentracingCfg(cfg ngx_config.Configuration) (string, error) { | |||
jsn := map[string]interface{}{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably just call this m
instead of jsn
, especially since it's not json here, even if it becomes it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
internal/ingress/controller/nginx.go
Outdated
jsn["sample_rate"] = *cfg.DatadogSampleRate | ||
} | ||
|
||
jsnBytes, err := json.Marshal(jsn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b
or buf
instead of jsnBytes
. The name barely carries meaning because it's a forced placeholder with a 5 line lifecycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
internal/ingress/controller/nginx.go
Outdated
func createOpentracingCfg(cfg ngx_config.Configuration) error { | ||
var tmpl *template.Template | ||
var fileContent string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its final purpose is to be the contents of a file, but at this stage it's conf
, config
or at a stretch, configData
The part that makes it fileContent
is the os.WriteFile
at the end, not what it is until that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
@@ -1001,8 +994,7 @@ func NewDefault() Configuration { | |||
DatadogEnvironment: "prod", | |||
DatadogCollectorPort: 8126, | |||
DatadogOperationNameOverride: "nginx.handle", | |||
DatadogSampleRate: 1.0, | |||
DatadogPrioritySampling: true, | |||
DatadogSampleRate: nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needed, except for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's here for consistency.
internal/ingress/controller/nginx.go
Outdated
} else { | ||
tmpl, _ = template.New("empty").Parse("{}") | ||
fileContent = "{}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to just return here and skip creating a file, or log some warning? That might be handled somewhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I'd prefer to leave it as it was. The current behavior is to produce a file containing {}
, you can even see it on disk in the container.
Thanks for reviewing, @cgilmour. I'll type up a description for a PR to propose upstream. |
kubernetes#10151 proposed upstream. |
Here's what I plan to propose in a PR upstream.
It uses the magic float-1.0
forsample_rate
to mean "use priority sampling instead," and then changes the default value to be-1.0
.It represents
DatadogSampleRate
as a*float32
instead offloat32
. This way, the default value ofnil
can be interpreted to mean "no rate specified." The configuration is decoded by themapstructure
package, which supports this style.In order to conditionally include
"sample_rate": ...
in/etc/nginx/opentracing.json
, I had to generalize the code that produces that file. I put the template-based implementation in a dedicated function, and that function is called for all tracers except Datadog. Datadog has its own function that uses theencoding/json
package instead oftext/template
.